Skip to content

Conversation

@SakiTakamachi
Copy link
Member

grapheme_split_string returns -1 in case of an error.

@SakiTakamachi SakiTakamachi force-pushed the fix/grapheme_levenshtein branch from 98d8c09 to 6ea4225 Compare May 1, 2025 13:07
@SakiTakamachi SakiTakamachi marked this pull request as ready for review May 1, 2025 13:45
@SakiTakamachi SakiTakamachi requested a review from devnexen as a code owner May 1, 2025 13:45
Copy link
Contributor

@youkidearitai youkidearitai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right. Make sense.

int32_t strlen_1, strlen_2;
strlen_1 = grapheme_split_string(ustring1, ustring1_len, NULL, 0);
strlen_2 = grapheme_split_string(ustring2, ustring2_len, NULL, 0);
if (UNEXPECTED(strlen_1 < 0 || strlen_2 < 0)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before merging, and since that s the whole point of this PR. can you think of few test cases to trigger this code path ? Cheers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devnexen

Logically, it seems to return -1 when a bad status is received, just like in the following location.

However, I can’t think of a specific way to intentionally reproduce that behavior…

if (U_FAILURE(ustatus)) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried something simpler, grapheme_strlen except U_MEMORY_ALLOCATION_ERROR I can t think of cases as bad inputs are caught up before that.

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself is correct and is unlikely to happen indeed. If no one else objects, feel free to merge.

@SakiTakamachi SakiTakamachi merged commit f1d259a into php:master May 2, 2025
9 checks passed
@SakiTakamachi SakiTakamachi deleted the fix/grapheme_levenshtein branch May 2, 2025 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants